Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: Support for RenderTargetBitmap, BitmapEncoder, SoftwareBitmap #7830

Merged
merged 10 commits into from
Jul 13, 2022
Merged

feat: Support for RenderTargetBitmap, BitmapEncoder, SoftwareBitmap #7830

merged 10 commits into from
Jul 13, 2022

Conversation

workgroupengineering
Copy link
Contributor

GitHub Issue (If applicable): closes #

PR Type

What kind of change does this PR introduce?

  • Feature

What is the current behavior?

What is the new behavior?

PR Checklist

Please check if your PR fulfills the following requirements:

Other information

Internal Issue (If applicable):

@gitpod-io
Copy link

gitpod-io bot commented Jan 15, 2022

This was referenced Jan 17, 2022
Closed
Closed
@jeromelaban
Copy link
Member

Very nice! You'll need to add System.Void Windows.Graphics.Imaging.BitmapEncoder::.ctor() as a method in the exclusion file (it's not a breaking change).

Also, can you add runtime tests to validate the behavior for these newly implemented classes ?

@workgroupengineering workgroupengineering force-pushed the features/Skia/RenderTargetBitmap branch 4 times, most recently from 6f83fc0 to 82ef727 Compare January 22, 2022 17:21
@workgroupengineering workgroupengineering marked this pull request as ready for review January 22, 2022 17:22
@workgroupengineering workgroupengineering requested a review from a team January 22, 2022 17:22
@workgroupengineering workgroupengineering force-pushed the features/Skia/RenderTargetBitmap branch from 82ef727 to a4655ed Compare January 23, 2022 11:48
@CLAassistant
Copy link

CLAassistant commented Jan 23, 2022 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@workgroupengineering workgroupengineering force-pushed the features/Skia/RenderTargetBitmap branch 5 times, most recently from 366c760 to a6e961a Compare January 24, 2022 10:07
@workgroupengineering workgroupengineering marked this pull request as draft January 24, 2022 10:51
@workgroupengineering
Copy link
Contributor Author

workgroupengineering commented Jan 24, 2022 •
edited
Loading

commit 076c10c will be removed when PR #7898 is merged.

@workgroupengineering
Copy link
Contributor Author

workgroupengineering commented Jan 24, 2022 •
edited
Loading

Hi @jeromelaban,
"Pack NuGet UWP" and "Pack NuGet WinUI" jobs fail because the reference assembly has a public ctor that the Skia runtime does not have. The official document BitmapEncoder does not have a public ctor.
How can I handle this situation?

@jeromelaban
Copy link
Member

For this situation, it's an acceptable breaking change and you can add the missing members, see this documentation.

@workgroupengineering workgroupengineering force-pushed the features/Skia/RenderTargetBitmap branch from dc2be17 to 702fb13 Compare January 27, 2022 09:17
@workgroupengineering workgroupengineering changed the title feat(Skia): RenderTargetBitmap,BitmapEncoder,SoftwareBitmap chore: Skia support for RenderTargetBitmap, BitmapEncoder, SoftwareBitmap Jan 27, 2022
@workgroupengineering workgroupengineering marked this pull request as ready for review January 27, 2022 09:19
@jeromelaban
Copy link
Member

Very nice updates! One last thing, can you add a runtime test which validates that the RenderTargetBitmap is actually producing the results we need ? (e.g. Render a blue Border and read a pixel in the middle, for instance).

@workgroupengineering workgroupengineering force-pushed the features/Skia/RenderTargetBitmap branch 2 times, most recently from 0f4e849 to e8dd7d3 Compare January 28, 2022 11:52
@workgroupengineering workgroupengineering marked this pull request as draft January 28, 2022 17:40
@workgroupengineering workgroupengineering force-pushed the features/Skia/RenderTargetBitmap branch from e8dd7d3 to f2db194 Compare February 1, 2022 17:13
@workgroupengineering workgroupengineering force-pushed the features/Skia/RenderTargetBitmap branch 2 times, most recently from 2ae122a to 3339b84 Compare June 6, 2022 13:47
@workgroupengineering workgroupengineering force-pushed the features/Skia/RenderTargetBitmap branch from 3339b84 to 8a531de Compare June 29, 2022 10:05
@workgroupengineering workgroupengineering force-pushed the features/Skia/RenderTargetBitmap branch 3 times, most recently from 595ee25 to 7b202db Compare July 8, 2022 07:32
@MartinZikmund
Copy link
Member

MartinZikmund commented Jul 8, 2022 •
edited
Loading

@workgroupengineering What is the state of this one, can be merged when the build passes? 😊

@workgroupengineering
Copy link
Contributor Author

workgroupengineering commented Jul 8, 2022 •
edited
Loading

@workgroupengineering What is the state of this one, can be merged when the build passes? 😊

WASM is not implemented, but I don't have the skills to do it.

@workgroupengineering workgroupengineering force-pushed the features/Skia/RenderTargetBitmap branch from 24bc2cd to 2736374 Compare July 8, 2022 16:48
@MartinZikmund
Copy link
Member

@workgroupengineering so can merge 🚀 ?

@workgroupengineering
Copy link
Contributor Author

@workgroupengineering so can merge 🚀 ?
yes you can

@MartinZikmund MartinZikmund enabled auto-merge July 13, 2022 07:44
@MartinZikmund MartinZikmund merged commit 674d344 into unoplatform:master Jul 13, 2022
@MartinZikmund
Copy link
Member

And merged!!! 🥳🥳🥳🥳🥳

@MartinZikmund
Copy link
Member

Awesome work! And thanks for the macOS runtime and snapshot tests! Now we just need to fix the failing runtime tests there and we will have much better coverage!

@MartinZikmund MartinZikmund mentioned this pull request Jul 13, 2022
Closed
6 tasks
@workgroupengineering workgroupengineering deleted the features/Skia/RenderTargetBitmap branch July 13, 2022 07:52
@MartinZikmund MartinZikmund mentioned this pull request Jul 13, 2022
Closed
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dr1rrb dr1rrb dr1rrb left review comments

@jeromelaban jeromelaban jeromelaban left review comments

@carldebilly carldebilly carldebilly approved these changes

@MartinZikmund MartinZikmund MartinZikmund approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@workgroupengineering @jeromelaban @CLAassistant @MartinZikmund @carldebilly @dr1rrb